-
Notifications
You must be signed in to change notification settings - Fork 86
Add role search to organization memberships #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
cf5c41b to
7f95689
Compare
| |> Repo.all | ||
|
|
||
| render(conn, "index.json-api", data: memberships) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the problem with this approach is that the two could be combined: you may be filtering either ids or roles.
I'm not sure what the actual requests look like that are generating the filtering, and if they're both used, but this does look like a bit of an explosion of endpoints and some duplication, as well as possibly missing combinations of request types.
|
You'll need to rebase off |
7f95689 to
34ba3ad
Compare
| |> where([om], om.role in ^roles) | ||
| _ -> | ||
| OrganizationMembership | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would personally simplify this into a couple different methods:
def index(conn, params) do
memberships = OrganizationMembership
|> with_filters(params)
|> preload([:organization, :member])
|> Repo.all
render(conn, "index.json-api", data: memberships)
end
defp with_filters(query, params) do
query
|> with_organization_filter(params)
|> with_role_filter(params)
|> with_member_filter(params)
end
defp with_organization_filter(query, %{"organization_id" => organization_id}) do
query |> where([om], om.organization_id == ^organization_id)
end
defp with_organization_filter(query, _), do: query
defp with_role_filter(query, %{"roles" => roles}) do
roles = roles |> coalesce_string
query |> where([om], om.role in ^roles)
end
defp with_role_filter(query, _), do: query
defp with_member_filter(query, %{"filter" => id_list}) do
ids = id_list |> coalesce_id_string
query |> where([om], om.member_id in ^ids)
end
defp with_member_filter(query, _), do: queryc75ee07 to
957e081
Compare
|
Looks like you have just one failure: |
|
Yeah, still need to edit/add tests |
342c901 to
4361c7c
Compare
4361c7c to
9d2fb39
Compare
| membership_2 = insert(:organization_membership, member: member_2) | ||
| insert(:organization_membership, member: member_3) | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit nitpicky, but mind ditching the extra empty line here? One is enough.
|
Other than that extra empty line, looks good to go. I would like to raise the question of code organization, though. I think we might have gotten to the point where Basically, everything in our controller helpers and most of model helpers fits together, maybe into something like a @joshsmith Do we want to create an issue for that? I think this is good to merge otherwise. If we want to reorganize, it should go with the separate issue. |
9d2fb39 to
952602e
Compare
|
@begedin yes let's please create a separate issue. |
Closes #132